Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Chat] 채팅 수정 API 구현 #166

Merged
merged 27 commits into from
Jun 12, 2024
Merged

Conversation

minisundev
Copy link
Member

#️⃣연관된 이슈

#165

📝작업 내용

채팅을 수정하는 기능을 구현했습니다

💬리뷰 요구사항

기존에 Repository를 Server의 Chat과 Room의 Chat을 나누면서
model도 따로따로 생성해서 사용했었는데 오늘 보니까 ServerChat과 RoomChat에서 다른것은 serverId/ roomId밖에 없길래 두 모델을 하나의 모델로 (Chat으로) 합쳤습니다

@minisundev minisundev added enhancement 추가 기능 API 상세 api 문서 Chat 채팅 관련 기능 labels Jun 10, 2024
@minisundev minisundev requested a review from a team June 10, 2024 09:12
@minisundev minisundev self-assigned this Jun 10, 2024
@minisundev minisundev changed the title Feat/edit chat [Chat] 채팅 수정 API 구현 Jun 10, 2024
Copy link
Contributor

@yudonggeun yudonggeun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

when (request.type) {
ChatType.Room -> chatService.updateRoomChat(request, userId)
ChatType.Server -> chatService.updateServerChat(request, userId)
else -> throw GlobalException(ErrorCode.INVALID_CHAT_TYPE)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

request 생성시에 invalid chat type 이라면 예외가 발생해서 이 코드는 히트하는 경우가 없을 것 같아요.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines 29 to 33
fun verifyAccess(userId: String) {
if (userId != this.userId) {
throw GlobalException(ErrorCode.FORBIDDEN_CHAT)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

내부 구현을 보지 않으면 무슨 검증인지 바로 알기는 힘들어보여서 좀 더 명확한 이름으로 수정하는 것은 어떨까요?

verifyAccess가 도메인에 어울리는 기능인가요? 서비스에 어울리는 기능인가요?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

넵 이름을 구체적으로 수정해보겠습니다!

Chat model이 다른데에서도 사용될 수 있다고 생각하고 재사용하는 경우를 생각하며 저렇게 짰는데 생각해보니 ChatService이외에는 쓰지 않을 것 같고 로직 자체는 Service단의 로직에 더 가까운 것 같아서 Service 단으로 옮기는 게 나을 것 같네요~!

Copy link
Contributor

@minahYu minahYu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

고생하셨습니다 👏👏

@minisundev minisundev merged commit db6b0d3 into kSideProject:dev Jun 12, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API 상세 api 문서 Chat 채팅 관련 기능 enhancement 추가 기능
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants